Change Flamegraph handling of narrow boxes (snap to multiples of 2 device pixels)#4601
Conversation
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks for the change! This looks definitely better than the current rendering where a lot of things are missing.
I left a few comments but I'm mostly OK landing this as is and improving this further later.
| export type ChartCanvasScale = { | ||
| // Always equal to devicePixelRatio | ||
| cssToDeviceScale: number, | ||
| // 1 if scaleCtxToCssPixels is true, otherwise equal to cssToDeviceScale | ||
| cssToUserScale: number, | ||
| }; |
There was a problem hiding this comment.
Is it useful to have both of them? I understand the rationale but I see it's used just to compare both to detect errors. Maybe just cssToUserScale is useful, because that's the one that actually changes when scaleCtxToCssPixels changes? But also scaleCtxToCssPixels is set by the caller anyway so the caller knows about that.
All in all I'm not convinced by adding this structure compared to just keep using devicePixelRatio directly. But I'm not against either so we can keep it :-)
There was a problem hiding this comment.
I'm undecided. Their "usefulness" comes into play when somebody reads the drawFilter method without having read the rest of the file; you can see at the start of the method which space you're rendering into.
| ) { | ||
| this._textMeasurement = new TextMeasurement(ctx); | ||
| this._textMeasurementCssToDeviceScale = cssToDeviceScale; | ||
| } |
There was a problem hiding this comment.
optional suggestion: use memoize-one with a function taking cssToDeviceScale as parameter. This would move the logic to invalidate the text measurement tool out of this function.
There was a problem hiding this comment.
Also this could possibly be handled directly by the shared Canvas code, passed to drawCanvas with the other properties. Because it looks like that the same code is repeated in all these charts.
There was a problem hiding this comment.
I agree, these suggestions would improve things! I'll leave them for follow-ups though.
| const { cssToUserScale } = scale; | ||
| if (cssToUserScale !== 1) { | ||
| throw new Error( | ||
| 'FlameGraphCanvasImpl sets scaleCtxToCssPixels={true}, so canvas user space units should be equal to CSS pixels.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Are these checks useful? After all, this component controls the prop scaleCtxToCssPixels, so is there a risk that in the future this might break?
(removing them is optional, I'm defering to your opinion, but I'm just curious about your thoughts)
There was a problem hiding this comment.
I'm less convinced that they're needed now, compared to when I was first reading this code. In the beginning I found it very counter-intuitive that drawCanvas would need to render into a completely different space depending on the value of a prop that was passed at the other end of the file. I wanted something locally inside drawCanvas that would let me orient myself (let me know which space I'm rendering into).
Now that I've looked at this code a bunch, I'm less convinced that this is such a big deal. But I'll keep it anyway. I think if we end up switching all charts to render in device pixels, we could remove these assertions and the cssToUserScale argument.
| // For each box, snap the left and right edges to the nearest multiple | ||
| // of two device pixels. If both edges snap to the same value, the box | ||
| // becomes empty and is not drawn. |
There was a problem hiding this comment.
I'm not sure of this. I think we should snap to at least 1px if there's a box, but do it just for one such small box on this px (if there are more than one box here), and only if there's no bigger box at this location.
This would be similar to (but not completely the same) what we do in the marker chart.
What do you think?
There was a problem hiding this comment.
Currently, when selecting a preview while viewing the flame graph on your deploy preview, we see "peaks" showing up and then disappearing very often. I don't know if this is the reason, but I'd expect the peaks to still show up even if they have a small width. It would be great if the view would stay mostly stable.
There was a problem hiding this comment.
It's true, both of these aspects are "cons" to the "just snap" approach. But I think they're outweighed by the "pros": The algorithm is simple and fast, and it never produces incorrect nesting: If box A is rendered below box B (in the same pixel column), you know that A is the caller of B. It seems hard to preserve peaks while also preserving correct nesting.
For what it's worth, Chrome's devtools performance panel also has flickering peaks as you zoom in and out.
In the past, the components which draw in device space were using window.devicePixelRatio and relying on the assumption that this is the same scale that ChartCanvas was using when it set the canvas backing size and CSS size. This assumption is correct, but it still feels better to pass the scale along explicitly, rather than assuming something about the global state. I'm also adding some checks so that it's obvious to readers of the drawCanvas methods how the canvas scale is controlled.
Fixes firefox-devtools#4591. This changes how small boxes and gaps are handled. If a row consists of many adjacent narrow boxes, we will now fill that entire row with painted pixels. Before this patch, we would skip all narrow boxes and leave large parts of the rendering empty.
mstange
force-pushed
the
flamegraph-snapping
branch
from
73c8658 to
2bf992e
Compare
2 participants
Fixes #4591.
Production | Deploy preview
I'm also changing some of the other charts for consistency. Please review each commit individually.
The last commit has a
snapfunction which says something about moving boxes past zero. This consistent snapping isn't really needed for the flame graph, but it could become relevant for the stack chart or the marker chart if we ever reuse this code there in the future.